Skip to content

Move longjmp into native code - #14188

Merged
aheejin merged 15 commits into
emscripten-core:mainfrom
aheejin:longjmp_native
Jun 3, 2021
Merged

Move longjmp into native code#14188
aheejin merged 15 commits into
emscripten-core:mainfrom
aheejin:longjmp_native

Conversation

@aheejin

@aheejin aheejin commented May 14, 2021

Copy link
Copy Markdown
Member

This moves longjmp and siglongjmp into native code and leaves only
the throwing stub in JS code, which cannot be moved. This makes code
simpler and makes all future wasm64-specific handling in JS side
unnecessary.

This currently fails a wasm2c test in test_longjmp_standalone. I don't
know enough about wasm2c but it looks wasm2c has its own support in
tools/wasm2c.py and tools/wasm2c/base.c. Any starting point on how to
fix that part?

This moves `longjmp` and `siglongjmp` into native code and leaves only
the throwing stub in JS code, which cannot be moved.

This currently fails a wasm2c test in `test_longjmp_standalone`. I don't
know enough about wasm2c but it looks wasm2c has its own support in
tools/wasm2c.py and tools/wasm2c/base.c. Any starting point on how to
fix that part?
@aheejin
aheejin requested review from kripken and sbc100 May 14, 2021 09:55
@aheejin

aheejin commented May 14, 2021

Copy link
Copy Markdown
Member Author

This was a part of #14108 and split into two parts.

@kripken kripken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the failing test, tools/wasm2c/base.c has an impl of Z_envZ_emscripten_longjmpZ_vii (emscripten_longjmp + wabt name mangling), which is used instead of the JS one. And now it will need to have an impl of the new _emscripten_throw_longjmp.

Comment thread tools/shared.py Outdated
Comment thread system/lib/libc/musl/include/setjmp.h Outdated
Comment thread tools/deps_info.py
@aheejin

aheejin commented May 21, 2021

Copy link
Copy Markdown
Member Author

test_SUPPORT_LONGJMP_object, which was added to test the situation in which SUPPORT_LONGJMP is set during compile time but not set at link time, fails now, presumably because the function to link does not exist in JS side anymore. Would this be a problem? Should we keep this test?

Comment thread emcc.py
# invoke calls, and they call setThrew(). We cannot handle this using
# deps_info as the invokes are not emitted because of library function
# usage, but by codegen itself.
'_setThrew',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem here is that you are introducing a new included-by-default function (since SUPPORT_LONGJMP defaults to 1).. so this will regress code size tests for tiny programs I believe. If a program never calls longjmp it should never need _setThrew... for exception handling the user has opted in so I more reasonable to accept that cost but most user don't explicitly opt into longjmp (they just get it because its the default).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it and added deps in deps_info.py instead with comments.

@sbc100

sbc100 commented May 21, 2021

Copy link
Copy Markdown
Collaborator

Indeed, it looks like a bunch of metadce tests are failing due to the new unconditional export.

@sbc100

sbc100 commented May 21, 2021

Copy link
Copy Markdown
Collaborator

test_SUPPORT_LONGJMP_object, which was added to test the situation in which SUPPORT_LONGJMP is set during compile time but not set at link time, fails now, presumably because the function to link does not exist in JS side anymore. Would this be a problem? Should we keep this test?

I think we should keep that test if we can.. presumably its a question of providing the right JS stub functions in the SUPPORT_LONGJMP == 0 case?

@aheejin

aheejin commented Jun 2, 2021

Copy link
Copy Markdown
Member Author

test_SUPPORT_LONGJMP_object, which was added to test the situation in which SUPPORT_LONGJMP is set during compile time but not set at link time, fails now, presumably because the function to link does not exist in JS side anymore. Would this be a problem? Should we keep this test?

I think we should keep that test if we can.. presumably its a question of providing the right JS stub functions in the SUPPORT_LONGJMP == 0 case?

Made this work too.

@aheejin

aheejin commented Jun 2, 2021

Copy link
Copy Markdown
Member Author

Sorry for the long delay. Hopefully all issues have been resolved now; PTAL.

@aheejin
aheejin merged commit aa2bad5 into emscripten-core:main Jun 3, 2021
@aheejin
aheejin deleted the longjmp_native branch June 3, 2021 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants